Make search more consistent #24121

bkamins · 2017-10-13T11:19:17Z

This is an implementation of #24103. If will fail the tests now. I will fix the tests when there is an agreement that the proposed functionality is OK.
All key assumptions how search now works are given in its docstring

nalimilan

There are too many unrelated formatting changes in this PR. Also I won't repeat comments I made at #24116. I'll review it again when it's more focused.

nalimilan · 2017-10-13T17:49:46Z

base/strings/search.jl

-or UTF-8 strings). The third argument optionally specifies a starting index. The return
-value is a range of indexes where the matching sequence is found, such that `s[search(s,x)] == x`:
+or UTF-8 strings).
+Alternatively the first and the second arguments may be arrays of `Int8` or `UInt8`.


I'd rather not document this, it really should be an internal function which shouldn't have been exposed. I was preparing a PR to merge search with findfirst which would also handle this.

nalimilan · 2017-10-13T17:50:45Z

base/strings/search.jl

+or UTF-8 strings).
+Alternatively the first and the second arguments may be arrays of `Int8` or `UInt8`.
+The third argument optionally specifies a starting index.
+If it is not a valid index into the first argument then error is thrown.


This goes without saying, I don't think it's worth making the description longer than it already is to mention this.

nalimilan · 2017-10-13T17:51:26Z

base/strings/search.jl

+Alternatively the first and the second arguments may be arrays of `Int8` or `UInt8`.
+The third argument optionally specifies a starting index.
+If it is not a valid index into the first argument then error is thrown.
+The return value when the second argument is a string or a vector of bytes is the samllest


"samllest". "exits".

nalimilan · 2017-10-13T17:52:34Z

base/strings/search.jl

+
+If the first argument is empty and third argument is not given `0:-1` is returned.
+
+The return value when the second argument is a signle character, a vector or a set of


nalimilan · 2017-10-13T17:54:03Z

base/strings/search.jl

-`search(string, 'c')` = `index` such that `string[index] == 'c'`, or `0` if unmatched.
+In particular a search for a zero length second argument returns `i:(i-1)`.
+
+If the first argument is empty and third argument is not given `0:-1` is returned.


Better give the general rule and use start=start(s) in the signature. People will figure out what happens in special cases.

nalimilan · 2017-10-13T17:55:39Z

base/strings/search.jl

-in(c::Char, s::AbstractString) = (search(s,c)!=0)
+search(s::AbstractString, c::Chars) = s == "" ? 0 : search(s, c, start(s))
+
+in(c::Char, s::AbstractString) = search(s,c) != 0


Plase don't change unrelated lines. Same below. This makes it really painful to review.

nalimilan · 2017-10-13T17:57:12Z

base/strings/search.jl

 function _search_bloom_mask(c)
    UInt64(1) << (c & 63)
 end

-_nthbyte(s::String, i) = codeunit(s, i)
-_nthbyte(a::ByteArray, i) = a[i]
+_nthbyte(s::String, i) = @inbounds codeunit(s, i)


Better add @inbounds at the call site, and add @propagate_inbounds annotations here.

nalimilan · 2017-10-13T17:57:54Z

base/strings/search.jl


-Similar to [`search`](@ref), but return only the start index at which
-the substring is found, or `0` if it is not.
+Similar to [`search`](@ref), but return only the start index at which the second argument


"substring" was clearer, let's not make the docs worse because of that UInt8 thing.

bkamins · 2017-10-15T21:57:30Z

@nalimilan Sorry for messing with the code formatting - lesson taken and I will fix it.

Before I correct this PR I would like a clear recommendation about the issue also discussed in #24116 regarding start argument of search.

Currently:

julia> search("", "asd")
0:-1

and I think this is what users would expect.
On the other hand:

julia> search("", "asd", 10)
0:-1

is something we want to avoid as 10 is an invalid index.

The only problem is what we want to return in search("", "asd") vs. search("", "asd", 1).
My reasoning is that the second form should throw an error. And I assume we have a consensus here as 1 is an invalid index for "". However, I think that search("", "asd") should still return 0:-1. Otherwise users will have to write their code as something like:

if endof(s) >= start(s)
    x = search(s, "some string")
    # do something with x
else
    # do the same thing knowing that no match was found
end

which is painful and error prone as in general user might not know if s is empty.

Remember that my proposal additionally fixes:

julia> search("","")
1:0

which is incorrect for sure. But I would say that 0:-1 should be returned (and not an error thrown in this case).

@nalimilan What do you think is a proper way to go given the above?

nalimilan · 2017-10-16T07:54:05Z

I hadn't realized it was such a Catch 22. I'd say we should have a special case for 1 with "", since start("") == 1, and start(s) is the default value for the index.

The only other solution I can think of is to allow search("", "asd") but not search("", "asd", 1), but that would be cumbersome to implement and problematic for users in they want to write generic code with an index falling back to start(s). So making an exception sounds better.

bkamins · 2017-10-16T08:15:09Z

Just to make sure I understand your sentence:

So making an exception sounds better.
You are opting to make search on "" always throw an exception (when searching for substrings, regexps and Chars)?

If this is the decision I can implement it and it should go into the documentation then.

nalimilan · 2017-10-16T08:26:58Z

That's my preferred solution at this stage, but you may want to way for possible comments at #24103 (comment).

bkamins · 2018-02-11T21:28:28Z

closing as it is outdated.

bkamins added 2 commits October 13, 2017 13:00

cleanup of 0 length str

6c8039f

add NEWS.md entry

eaa4f45

bkamins mentioned this pull request Oct 13, 2017

String search inconsistency #24103

Open

fix invalid ranges

3df5b06

nalimilan reviewed Oct 13, 2017

View reviewed changes

kshyatt added the strings "Strings!" label Oct 16, 2017

bkamins closed this Feb 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make search more consistent #24121

Make search more consistent #24121

bkamins commented Oct 13, 2017

nalimilan left a comment

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

nalimilan Oct 13, 2017

bkamins commented Oct 15, 2017

nalimilan commented Oct 16, 2017

bkamins commented Oct 16, 2017

nalimilan commented Oct 16, 2017

bkamins commented Feb 11, 2018


		If the first argument is empty and third argument is not given `0:-1` is returned.

		The return value when the second argument is a signle character, a vector or a set of

Make search more consistent #24121

Make search more consistent #24121

Conversation

bkamins commented Oct 13, 2017

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Oct 15, 2017

nalimilan commented Oct 16, 2017

bkamins commented Oct 16, 2017

nalimilan commented Oct 16, 2017

bkamins commented Feb 11, 2018